[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

HannesWell · 2024-09-17T22:43:48Z

As said in #423 (comment) this is the current state of my stalled work to migrate E4 Xpath off the old and unmaintained apache.commons.jxpath library.
The basic idea is to provide a org.w3c.dom.Element view/wrapper for an EObject so that an javax.xml.xpath.Xpath can operate on it.
As mentioned this is heavily work in progres, not yet functional and a lot has to be cleaned up before this can be used (it contains a lot of try out code).

@ptziegler if you or anybody else like to take this over and complete it please feel free. I also would find this an interesting topic, but I have currently no time to work on this. But if you don't have time either, I might continue this by myself in the future.

I have also extracted some minor improvements that can be applied now already in #2289.

Fixes #423

github-actions · 2024-09-17T23:28:33Z

Test Results

1 818 files - 3 1 818 suites - 3 1h 35m 36s ⏱️ + 7m 43s
7 719 tests - 4 7 491 ✅ - 4 228 💤 ±0 0 ❌ ±0
24 318 runs - 12 23 569 ✅ - 12 749 💤 ±0 0 ❌ ±0

Results for commit cc7ae8a. ± Comparison against base commit c9f129d.

This pull request removes 4 tests.

org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_getProperty
org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_getPropertyNames
org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_getProperty_When_UnknownProperty_Expect_NullResult
org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_setProperty

♻️ This comment has been updated with latest results.

ptziegler · 2024-09-23T08:05:46Z

I played around with this draft and I think the initial approach can work, with the main obstacles being:

jxpath has the application as root, while xpath has the document. This leads to weird situation where the XPath "/" has to be translated to "/application/".
The handling of the parent context is something that needs to be extensively tested. Also whether e.g. "/" is now the root of the parent context. I'm also not sure how this would be encoded in XML. I.e. whether both documents need to be merged or whether the "child" application needs to be appended to the "parent" application or if it's something completely different.

How should we proceed here? I don't think I can push directly to your branch. So should I create a separate branch where I do my own development on?

HannesWell · 2024-09-23T21:09:33Z

I played around with this draft and I think the initial approach can work,

Awesome!

with the main obstacles being:

jxpath has the application as root, while xpath has the document. This leads to weird situation where the XPath "/" has to be translated to "/application/".

Couldn't this be fixed by adding a placeholder/virtual/dummy document?

The handling of the parent context is something that needs to be extensively tested. Also whether e.g. "/" is now the root of the parent context. I'm also not sure how this would be encoded in XML. I.e. whether both documents need to be merged or whether the "child" application needs to be appended to the "parent" application or if it's something completely different.

I cannot say much about this a.t.m.

How should we proceed here? I don't think I can push directly to your branch. So should I create a separate branch where I do my own development on?

Yes you have to create your own branch and PR, but you could add a link to this. If you have created it, this can be closed.

laeubi · 2024-09-24T04:34:56Z

I'm also not sure how this would be encoded in XML. I.e. whether both documents need to be merged or whether the "child" application needs to be appended to the "parent" application or if it's something completely different.

Can you explain what exactly is the problem/question?

jxpath has the application as root, while xpath has the document. This leads to weird situation where the XPath "/" has to be translated to "/application/"

I would expect that application is the document element or do I understand the problem wrong?

ptziegler · 2024-09-24T04:55:17Z

Can you explain what exactly is the problem/question?

I'm simply not sure how the parent context is handled in jxpath. But until we can properly ready the current context, this doesn't have a very high priority on my side.

I would expect that application is the document element or do I understand the problem wrong?

Given the following XML document:

<foo>
    <bar/>
    <bar/>
    <bar/>
</foo>

When converted to a Java document, you get the following object structure:

- Document
  - Element (foo)
    - Element (bar)
    - Element (bar)
    - Element (bar)

Evaluating the XPath "/" on any node returns Document and not Element(foo).

laeubi · 2024-09-24T05:07:16Z

If I understand right we already implement the DOM API here (maybe something better placed at EMF directly? @merks ?) so can't Application implement Document + Element here and simplify return it as document and the root element (might be a bit counterintuitive but probably works).

merks · 2024-09-24T05:35:05Z

Stop to ask, why are there so many alternatives to DOM? (Because it's horrible?!)

Goodness knows why folks could not have just use EMF's support for paths?

org.eclipse.emf.ecore.resource.impl.ResourceImpl.getEObject(List)
org.eclipse.emf.ecore.InternalEObject.eObjectForURIFragmentSegment(String)

Probably wasn't pretty enough? Not standard enough? Note powerful enough? Best to hide EMFness?

In any case, no one ever asked me for advice or suggestions, so I have no clue how it was necessary to have the full power of XPath available to reference an object when there are far simpler mechanisms available for doing just that.

I definitely don't want to push this problem down into EMF. People have asked for many things, but never this thing.

laeubi · 2024-09-24T06:14:21Z

Stop to ask, why are there so many alternatives to DOM? (Because it's horrible?!)

EMF is a DOM as well, it just don't implement the (XML) DOM API ;-)

Probably wasn't pretty enough? Not standard enough? Note powerful enough? Best to hide EMFness?

I have no clue but can only assume because the e4 xmi is actually an XML document and XPath is the standard for XML .. anyways Xpath itself do not mandates to use DOM, it supports other (xml) representations as well, thats why I previously mentioned that we probably just need to copy the parser part, because in the end we only need to parse an Xpath Expression and map it to the (EMF) DOM thats what actually is done as of today.

Sadly I have found little to no documentation on this feature so its quite hard to guess what must be supported and how exactly it is mapped or what where the reasons for a design decision. Also the UI for this is really bare....

mickaelistria · 2024-09-24T07:22:03Z

Probably wasn't pretty enough? Not standard enough? Note powerful enough? Best to hide EMFness?

I'm pretty sure it's just because XPath is standard and popular enough to assume most developers will feel comfortable enough with it for this case.
I'm wondering whether the full power of XPath is required here. Looking at all found instances on GitHub https://github.com/search?q=path%253A*.e4xmi+xpath&type=code&ref=advsearch , we can see only a few basic patterns: xpath:/ (root, 95% of occurrences), xpath://mainMenu (select all), xpath:/mainMenu/child[1], xpath://*[@elementId='fragment.contributedMenu1' or @elementId='fragment.contributedMenu2'] (select by attribute). We could consider just sticking to supporting that list.

If EMF already support well an XPath-like syntax to select node and this syntax is xpath enough to expect most users wouldn't need to change their extensions to get the same node selected, we could consider just dropping XPath and adopting the EMF way.
If the is another close but not directly complatible syntax supported natively in EMF, we could consider converting XPath to this syntax.
@merks What do you think is possible/best here to rely on more native EMF features?

merks · 2024-09-24T07:50:55Z

The XPath library being used has the benefit that it operates on any DOM-like structure. The built-in XPath support works only on org.w3c.dom. That's simply nasty such that one must try to serialize the model to a DOM and keep a mapping to work your way back. I haven't looked at the details of prototype. It's not clear to me that cloning jxpath and deleting the unused content would not be the easier approach. Either way, there is a whole whack of complex crap that needs to be maintained...

I think at this point, we are stuck needing to support XPath expressions exactly as they are current used, so we must parse them and evaluate them somehow. Alternative approaches are water under the bridge that can't be pushed back upstream. (I bring it up merely because I do not want EMF, i.e., me personally, to burdened with this, but I'm happy to help the Platform wherever I can.)

ptziegler · 2024-09-24T08:40:42Z

It's not clear to me that cloning jxpath and deleting the unused content would not be the easier approach. Either way, there is a whole whack of complex crap that needs to be maintained...

I believe both approaches are feasible but at least in the long term, we should try to remove the reference JXPath. But given that this will take quite a lot of effort, it also makes sense to simply fork the JXPath project until then.
Perhaps we also come to the conclusion that the XPath approach doesn't work out as well as we had hoped, in which case we still have the alternative to fall back on. I'll try to draft a PR for the fork, separately from the PR for using XPath.

merks · 2024-09-24T09:16:49Z

FYI, in Orbit I build axis1 (horrible but BIRT uses it) from source and publish it to repo.eclipse.org so that we can use BND to create an OSGi build from it as if it were published to Maven central:

https://github.com/eclipse-orbit/orbit-simrel/blob/main/maven-deploy/MavenAxis.jenkinsfile

We could do that with jxpath, or a fork of jxpath, perhaps a fork where only the "CVE" functionality is disabled so that there really isn't much to maintain at all, and it could be rebased on newer versions of jxpath in the future.

Just a thought...

ptziegler · 2024-09-24T09:27:13Z

We could do that with jxpath, or a fork of jxpath, perhaps a fork where only the "CVE" functionality is disabled so that there really isn't much to maintain at all, and it could be rebased on newer versions of jxpath in the future.

That's effectively the case with org.apache.commons.jxpath v1.3.0.v200911051830 that was used previously. Because this plugin doesn't import e.g. the javax.servlet packages, all of the "remote execution" CVEs are effectively irrelevant, as the application would already fail with an exception, when trying to initialize the servlets.

laeubi · 2024-09-24T09:29:59Z

Because this plugin doesn't import e.g. the javax.servlet packages, all of the "remote execution" CVEs are effectively irrelevant, as the application would already fail with an exception, when trying to initialize the servlets.

As jxpath does not run inside a servelt / EE Container, they are effectively irrelevant for where we use that as well ... ;-)

In any case, embedding the code seem more suitable than building something that is similar but named the same as an official artifact.

HannesWell · 2024-10-08T16:39:41Z

Today I stumbled upon the jaxen library, which says:

The Jaxen XPath Engine for Java
[...]
It is also possible to write adapters that treat non-XML trees such as compiled Java byte code
or Java beans as XML, thus enabling you to query these trees with XPath too.

It sounds like this maybe could be an alternative for jXpath. It isn't very active either but it's latest release is only two years old.
And the good thing is, it's already in simrel-Orbit and seems to have no extra dependencies https://github.com/eclipse-orbit/orbit-simrel/blob/75cc23701b2417f530efd3ce51763aef09c5a206/maven-osgi/tp/Maven.target#L678-L680

ptziegler · 2024-10-08T17:46:31Z

Today I stumbled upon the jaxen library, which says:
The Jaxen XPath Engine for Java
[...]
It is also possible to write adapters that treat non-XML trees such as compiled Java byte code
or Java beans as XML, thus enabling you to query these trees with XPath too.
It sounds like this maybe could be an alternative for jXpath. It isn't very active either but it's latest release is only two years old. And the good thing is, it's already in simrel-Orbit and seems to have no extra dependencies https://github.com/eclipse-orbit/orbit-simrel/blob/75cc23701b2417f530efd3ce51763aef09c5a206/maven-osgi/tp/Maven.target#L678-L680

I gave it a quick try, but I don't believe it works as well as it should... For example, you can't "skip" nodes, so expressions like "children/mainMenu" work, but "//mainMenu" doesn't. Getting the current object via "/" also doesn't work...

akurtakov · 2025-01-14T05:47:40Z

Is there progress here? It would be nice to get rid of jxpath and it's CVEs this cycle.

eclipse-platform-bot · 2025-01-14T05:51:53Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From b6e9b024238621c74b3996faaac3f3e1d438e66d Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 14 Jan 2025 05:51:29 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF
index 74cc46b704..0fd771831e 100644
--- a/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Bundle-Name
 Bundle-SymbolicName: org.eclipse.e4.emf.xpath
-Bundle-Version: 0.5.0.qualifier
+Bundle-Version: 0.5.100.qualifier
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Require-Bundle: org.eclipse.emf.ecore;bundle-version="2.35.0",
  org.eclipse.core.runtime;bundle-version="3.29.0"
-- 
2.47.1

Further information are available in Common Build Issues - Missing version increments.

ptziegler · 2025-01-14T08:47:01Z

Is there progress here? It would be nice to get rid of jxpath and it's CVEs this cycle.

Sadly not, at least from my side. A lot of stuff has been piling up over the past months and I fear I won't be able to work on this anytime soon. 😦

laeubi · 2025-01-14T09:51:43Z

@akurtakov @HannesWell @ptziegler I have now just taken the opportunity to try the path to embed jxpath here:

Embedd jxpath #2691

...lipse.e4.emf.xpath.test/src/org/eclipse/e4/emf/xpath/test/ExampleQueriesApplicationTest.java

ptziegler · 2025-01-18T20:02:21Z

...pse.e4.emf.xpath/src/org/eclipse/e4/emf/xpath/internal/java/JavaXPathContextFactoryImpl.java

+			EObjectContext parent = ((EObjectContext) parentContext);
+			Element rootElement = parent.object2domProxy.get(contextBean);
+			if (rootElement == null) {
+				throw new IllegalArgumentException("Context bean is not from the same tree its parent context");


This is perfectly valid in JXPath, so this must not throw an exception. More generally, I think we both have a general misunderstanding on how the parent context translates to the XML structure.

Documentation is really sparse, at best. But as far as I understand it, the parent context is only used to share the configuration. Meaning the locale, variables etc.
But this doesn't mean that the XML document of the child context is contained by the XML document of the parent context.

For example, this is the test case I had extended for my implementation:

/******************************************************************************* * Copyright (c) 2018, 2025 vogella GmbH and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 * which accompanies this distribution, and is available at * https://www.eclipse.org/legal/epl-2.0/ * * SPDX-License-Identifier: EPL-2.0 * * Contributors: * Lars Vogel <[email protected]> - initial contribution ******************************************************************************/ package org.eclipse.e4.emf.xpath.test; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import org.eclipse.e4.emf.xpath.EcoreXPathContextFactory; import org.eclipse.e4.emf.xpath.XPathContext; import org.eclipse.e4.emf.xpath.XPathContextFactory; import org.eclipse.e4.ui.internal.workbench.E4XMIResourceFactory; import org.eclipse.e4.ui.model.application.MAddon; import org.eclipse.e4.ui.model.application.MApplication; import org.eclipse.e4.ui.model.application.commands.impl.CommandsPackageImpl; import org.eclipse.e4.ui.model.application.impl.ApplicationPackageImpl; import org.eclipse.e4.ui.model.application.ui.advanced.impl.AdvancedPackageImpl; import org.eclipse.e4.ui.model.application.ui.basic.impl.BasicPackageImpl; import org.eclipse.e4.ui.model.application.ui.impl.UiPackageImpl; import org.eclipse.e4.ui.model.application.ui.menu.MMenu; import org.eclipse.e4.ui.model.application.ui.menu.impl.MenuPackageImpl; import org.eclipse.e4.ui.model.fragment.MModelFragments; import org.eclipse.e4.ui.model.fragment.MStringModelFragment; import org.eclipse.emf.common.util.URI; import org.eclipse.emf.ecore.EObject; import org.eclipse.emf.ecore.resource.Resource; import org.eclipse.emf.ecore.resource.ResourceSet; import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl; import org.junit.After; import org.junit.Before; import org.junit.Test; public class ExampleQueriesApplicationTest { private ResourceSet resourceSet; private XPathContext xpathContext; private XPathContext xpathChildContext; private Resource resource; private Resource childResource; @SuppressWarnings("restriction") @Before public void setUp() { resourceSet = new ResourceSetImpl(); resourceSet.getResourceFactoryRegistry().getExtensionToFactoryMap() .put(Resource.Factory.Registry.DEFAULT_EXTENSION, new E4XMIResourceFactory()); resourceSet.getPackageRegistry().put(ApplicationPackageImpl.eNS_URI, ApplicationPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(CommandsPackageImpl.eNS_URI, CommandsPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(UiPackageImpl.eNS_URI, UiPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(MenuPackageImpl.eNS_URI, MenuPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(BasicPackageImpl.eNS_URI, BasicPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(AdvancedPackageImpl.eNS_URI, AdvancedPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put( org.eclipse.e4.ui.model.application.descriptor.basic.impl.BasicPackageImpl.eNS_URI, org.eclipse.e4.ui.model.application.descriptor.basic.impl.BasicPackageImpl.eINSTANCE); URI uri = URI.createPlatformPluginURI("/org.eclipse.e4.emf.xpath.test/model/Application.e4xmi", true); resource = resourceSet.getResource(uri, true); XPathContextFactory<EObject> f = EcoreXPathContextFactory.newInstance(); xpathContext = f.newContext(resource.getContents().get(0)); URI childUri = URI.createPlatformPluginURI("/org.eclipse.e4.emf.xpath.test/model/fragment.e4xmi", true); childResource = resourceSet.getResource(childUri, true); xpathChildContext = f.newContext(xpathContext, childResource.getContents().get(0)); } @After public void tearDown() { xpathContext = null; resource.unload(); resourceSet.getResources().remove(resource); xpathChildContext = null; childResource.unload(); resourceSet.getResources().remove(childResource); } @Test public void testAccessingTheApplication() { Object application = xpathContext.getValue("/"); assertThat(application).isInstanceOf(MApplication.class); } @Test public void testAccessingTheMainMenu() { Object menu = xpathContext.getValue("//mainMenu"); assertThat(menu).isInstanceOf(MMenu.class); MMenu mMenu = xpathContext.getValue("//mainMenu", MMenu.class); assertNotNull(mMenu); Object menu2 = xpathContext.getValue("/children/mainMenu"); assertThat(menu2).isInstanceOf(MMenu.class); } @Test public void testAccessingAllMenus() { Object menuEntries = xpathContext.getValue("//mainMenu/children"); assertNotNull(menuEntries); assertThat(menuEntries).isInstanceOf(MMenu.class); assertEquals("File", ((MMenu) menuEntries).getLabel()); } @Test public void testAccessingTheModelFragments() { Object modelFragments = xpathChildContext.getValue("/"); assertThat(modelFragments).isInstanceOf(MModelFragments.class); } @Test public void testAccessingTheStringModelFragment() { Object modelFragment = xpathChildContext.getValue("//fragments[1]"); assertThat(modelFragment).isInstanceOf(MStringModelFragment.class); MStringModelFragment mModelFragment = xpathChildContext.getValue("//fragments[1]", MStringModelFragment.class); assertNotNull(mModelFragment); Object modelFragment2 = xpathChildContext.getValue("/fragments[1]"); assertThat(modelFragment2).isInstanceOf(MStringModelFragment.class); } @Test public void testAccessingTheAddons() { Object addon = xpathChildContext.getValue("//elements[1]"); assertThat(addon).isInstanceOf(MAddon.class); MAddon mAddon = xpathChildContext.getValue("//elements[1]", MAddon.class); assertNotNull(mAddon); } }

with the fragment.e4xmi:

<?xml version="1.0" encoding="ASCII"?> <fragment:ModelFragments xmi:version="2.0" xmlns:xmi="http://www.omg.org/XMI" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:application="http://www.eclipse.org/ui/2010/UIModel/application" xmlns:fragment="http://www.eclipse.org/ui/2010/UIModel/fragment" xmi:id="_G1t40NXTEe-i_c0uGwOUng"> <fragments xsi:type="fragment:StringModelFragment" xmi:id="_H8jlENXTEe-i_c0uGwOUng" featurename="addons" parentElementId="xpath:/"> <elements xsi:type="application:Addon" xmi:id="_Je0CANXTEe-i_c0uGwOUng" elementId="org.eclipse.e4.emf.xpath.test.addon.0"/> </fragments> </fragment:ModelFragments>

Note how xpathChildContext.getValue("/") returns an instance of MModelFragments, even though it has the application context as its parent?

This is perfectly valid in JXPath, so this must not throw an exception.

I found another existing use-case were I got that impression as well. The reason why this is exists, because of this test-case:

eclipse.platform.ui/tests/org.eclipse.e4.emf.xpath.test/src/org/eclipse/e4/emf/xpath/test/ExampleQueriesTestCase.java

Line 84 in ef4c5db

assertThrows(JXPathNotFoundException.class, () -> xpathContext.getValue(".[@id='nixda']"));

But probably an exception is only thrown in a more specific case and not generally if the match result is empty.

More generally, I think we both have a general misunderstanding on how the parent context translates to the XML structure.

Documentation is really sparse, at best. But as far as I understand it, the parent context is only used to share the configuration. Meaning the locale, variables etc. But this doesn't mean that the XML document of the child context is contained by the XML document of the parent context.

I fully agree on the first part and that's for example why the XPath object is shared in the current state of this PR, but I'm not sure about the latter. If parent-context is shared but not the XML document that coupling seems very loose. I cannot really tell if that generality is needed.
But yes the documentation is indeed really sparse... I'll check the issue in whose context this was added, maybe that provides more info:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=324954

In general I wonder if the parent context capability is even used, respectively if this E4 Xpath is used through it's java API at all or only in E4-model(fragments)? Because AFAICT the latter never uses a parent context.
If possible and for simplicity we could remove the parent-context handling too?

For example, this is the test case I had extended for my implementation:

Please contribute this extended test in a separate PR, we really need more tests on this topic. So if you have more cases you know that should be tested please add them as well. My problem is that I have almost no real-life use-case I can use to test this change.

Yes, I really don't think relative paths make much sense and in the Platform's own restricted use cases the context is always an MApplication. There are places that already handle "/" as a degenerate simple case:

I don't find a single xpath expression that does not start with xpath:/:

And even if there were, it's evaluated against the MApplication so I think would be the same as prefixing a / wouldn't it?

Does it make the problem simpler to assume that only absolute paths are used starting with either / or // and to simply reject relatively paths or simply prefix them with /? Then we could simply map xpath:/[^/] to /Application/... couldn't we?

This is perfectly valid in JXPath, so this must not throw an exception.

You are right. And I investigated the test-case that caused me to implement it and it's actually that just the XPathContext.getValue() method that throws an exception if an element is not found. But calling XPathContext.iterate/stream() with that xpath would yield an empty iterator/stream instead.

I extended the test-case to make this finding more clear in #2784.

In #2784, I also added your test-case in a commit were I set you as author.

Documentation is really sparse, at best. But as far as I understand it, the parent context is only used to share the configuration. Meaning the locale, variables etc.
But this doesn't mean that the XML document of the child context is contained by the XML document of the parent context.

From what you and Ed have described and looking at the old impl, I have also the impression that this is right. The only challenge a corresponding change is causing is that in the lambda passed to xpath.setXPathFunctionResolver() the context respectively it's Ecore<->Dom-mapping is referenced.
In the suggestions Ed made in #2775 (thank you for that!) the function-resolver is simply overridden, but if different documents are used and parent and children are used alternately, this causes problems.
This could be worked-around by setting the function-resolver on each evaluation of the xpath (which wouldn't be thread-safe) or we could use a ThreadLocal that is set on each evaluation. But since the documentation javax.xml.xpath.Xpath says: An XPath object is not thread-safe and not reentrant. thread-safety, even between parent- and child-context is nothing we have to worry about.

But when looking at the implementation of javax.xml.xpath.Xpath I generally wonder which kind of state we are actually trying to share. Because we only field we set is the function-resolver (which we actually don't want to fully share as explained above).
And I wonder how variables mentioned in the documentation are actually intended to be set. I don't see a way to do that?

eclipse.platform.ui/bundles/org.eclipse.e4.emf.xpath/src/org/eclipse/e4/emf/xpath/XPathContextFactory.java

Lines 34 to 36 in c9f129d

* Creates a new XPathContext with the specified bean as the root node and the

* specified parent context. Variables defined in a parent context can be

* referenced in XPaths passed to the child context.

Do we want to set a XPathVariableResolver as well? But through which API? I don't want to add this (here).
When just ignoring the parent context, most (maybe even all, some failures might have other causes) seem to pass still.

The only strong reason to re-use the Xpath I see currently is a case from Ed's test extension in
https://github.com/eclipse-platform/eclipse.platform.ui/pull/2775/files#diff-08f967627c7475d3f8720ef977462e017834ddf6e88dd796acef9c53acf797cc

List<Node> followingSiblingsList = nestedXpathContext.stream("following-sibling::*", Node.class).toList(); assertEquals(eContents.subList(1, eContents.size()), followingSiblingsList);

But then having a different document doesn't seem to make sense to me.

HannesWell · 2025-02-10T20:43:11Z

I have now added another workaround to reconstruct and return a list-value in XPathContext.getValue() if the specified xpath has a many-reference as terminal so that the last test-case also passes without changes:

eclipse.platform.ui/tests/org.eclipse.e4.emf.xpath.test/src/org/eclipse/e4/emf/xpath/test/ExampleQueriesApplicationTest.java

Lines 105 to 113 in c9f129d

    
           public void testAccessingAllMenus() { 
        
           	Object menuEntries = xpathContext.getValue("//mainMenu/children"); 
        
           	assertThat(menuEntries).isInstanceOf(List.class); 
        
           	List<?> list = (List<?>) menuEntries; 
        
           	assertEquals(2, list.size()); 
        
           	assertThat(list).allMatch(MMenu.class::isInstance, "Is instanceof of MMenu") // 
        
           			.anyMatch(e -> "File".equals(((MMenu) e).getLabel())) 
        
           			.anyMatch(e -> "Help".equals(((MMenu) e).getLabel())); 
        
           }

With this all tests pass and I think this is ready.
Although I'm still not sure if the parent-context handling has any real use-case as explained in: #2290 (comment)
I'll create another PR suggesting to deprecate that and other parts of this API for removal, where we could further discuss .

merks

Just one minor improvement.

Otherwise it's great to see such a simple solution!

...pse.e4.emf.xpath/src/org/eclipse/e4/emf/xpath/internal/java/JavaXPathContextFactoryImpl.java

Fixes eclipse-platform#423 Co-authored-by: Patrick Ziegler <[email protected]> Co-authored-by: Ed Merks <[email protected]>

HannesWell · 2025-02-11T08:53:34Z

The build is completely green now and all tests pass.

If no objections are raised, I'll submit this PR this (European) evening.

merks · 2025-02-11T09:05:52Z

No objections. Why not push the big green button now?

iloveeclipse · 2025-02-11T10:21:25Z

Sooner is better. I would even trigger a new build immediately.

iloveeclipse · 2025-02-11T10:22:07Z

https://ci.eclipse.org/releng/job/Builds/job/I-build-4.35/123/ triggered

merks · 2025-02-11T10:50:15Z

Thanks 🙏

ptziegler · 2025-02-11T21:27:47Z

Apologies for the late response. I really like just how much cleaner this looks now, so no objections from my side, either.

HannesWell · 2025-02-11T22:53:07Z

No objections. Why not push the big green button now?

I just wanted to give other the opportunity for a few more hours to express their opinion. But I'm also fine to have it sooner. :)

Apologies for the late response. I really like just how much cleaner this looks now, so no objections from my side, either.

Great. :)

Relates to - eclipse-platform/eclipse.platform.ui#423 - eclipse-platform/eclipse.platform.ui#2290 - eclipse-platform/eclipse.platform.ui#2789

HannesWell mentioned this pull request Sep 17, 2024

Remove dependencies on org.apache.commons.jxpath #423

Closed

HannesWell force-pushed the javax.xpath branch from 3e0d31e to a8e0384 Compare September 23, 2024 20:26

ptziegler mentioned this pull request Oct 11, 2024

[WIP] [E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2387

Closed

akurtakov force-pushed the javax.xpath branch from a8e0384 to c8fea16 Compare January 14, 2025 05:47

HannesWell mentioned this pull request Jan 14, 2025

Embedd jxpath #2691

Closed

HannesWell force-pushed the javax.xpath branch from 443239a to bdf11d8 Compare January 17, 2025 23:13

HannesWell changed the title ~~[WIP] [E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath~~ [E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath Jan 18, 2025

HannesWell force-pushed the javax.xpath branch from bdf11d8 to 063ed05 Compare January 18, 2025 19:15

ptziegler reviewed Jan 18, 2025

View reviewed changes

merks force-pushed the javax.xpath branch from 063ed05 to 32169ea Compare February 5, 2025 08:56

HannesWell mentioned this pull request Feb 9, 2025

Extend E4 xpath tests #2784

Merged

HannesWell force-pushed the javax.xpath branch from 32169ea to 6ab1980 Compare February 9, 2025 20:09

HannesWell mentioned this pull request Feb 9, 2025

Remove dependencies on org.apache.commons.jxpath #2775

Closed

HannesWell force-pushed the javax.xpath branch 3 times, most recently from d75b9cd to 5cf3dfe Compare February 10, 2025 20:37

HannesWell marked this pull request as ready for review February 10, 2025 20:43

HannesWell requested review from merks and ptziegler February 10, 2025 20:43

HannesWell mentioned this pull request Feb 10, 2025

[E4 XPath] Add new API in EModelService to find elements via Xpath and deprecate E4-XPath bundle for removal #2789

Merged

merks approved these changes Feb 11, 2025

View reviewed changes

...pse.e4.emf.xpath/src/org/eclipse/e4/emf/xpath/internal/java/JavaXPathContextFactoryImpl.java Outdated Show resolved Hide resolved

HannesWell and others added 2 commits February 11, 2025 09:02

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath

1052e3c

Fixes eclipse-platform#423 Co-authored-by: Patrick Ziegler <[email protected]> Co-authored-by: Ed Merks <[email protected]>

Initial support for parent context-handling

cc7ae8a

HannesWell force-pushed the javax.xpath branch from 5cf3dfe to cc7ae8a Compare February 11, 2025 08:02

iloveeclipse merged commit 35844b5 into eclipse-platform:master Feb 11, 2025
17 checks passed

HannesWell deleted the javax.xpath branch February 11, 2025 22:33

HannesWell mentioned this pull request Feb 16, 2025

Add info about E4 XPath re-implementation and deprecation eclipse-platform/www.eclipse.org-eclipse#276

Merged

	* Creates a new XPathContext with the specified bean as the root node and the
	* specified parent context. Variables defined in a parent context can be
	* referenced in XPaths passed to the child context.

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

Uh oh!

Conversation

HannesWell commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

ptziegler commented Sep 23, 2024

Uh oh!

HannesWell commented Sep 23, 2024

Uh oh!

laeubi commented Sep 24, 2024

Uh oh!

ptziegler commented Sep 24, 2024

Uh oh!

laeubi commented Sep 24, 2024

Uh oh!

merks commented Sep 24, 2024

Uh oh!

laeubi commented Sep 24, 2024

Uh oh!

mickaelistria commented Sep 24, 2024

Uh oh!

merks commented Sep 24, 2024

Uh oh!

ptziegler commented Sep 24, 2024

Uh oh!

merks commented Sep 24, 2024

Uh oh!

ptziegler commented Sep 24, 2024

Uh oh!

laeubi commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HannesWell commented Oct 8, 2024

Uh oh!

ptziegler commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akurtakov commented Jan 14, 2025

Uh oh!

eclipse-platform-bot commented Jan 14, 2025

Uh oh!

ptziegler commented Jan 14, 2025

Uh oh!

laeubi commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ptziegler Jan 18, 2025

Choose a reason for hiding this comment

Uh oh!

HannesWell Jan 19, 2025

Choose a reason for hiding this comment

Uh oh!

merks Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

HannesWell Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

HannesWell Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

HannesWell Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

HannesWell commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HannesWell commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

HannesWell commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading

laeubi commented Sep 24, 2024 •

edited

Loading

ptziegler commented Oct 8, 2024 •

edited

Loading

laeubi commented Jan 14, 2025 •

edited

Loading

HannesWell commented Feb 10, 2025 •

edited

Loading

HannesWell commented Feb 11, 2025 •

edited

Loading